-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Do not run lintcheck summary on feature-freezed PRs #15272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't a better fix look for the proper lintcheck comment using the API and replace it, rather than edit the latest comment done by the bot whatever it is? Or, simpler, post a new comment with lintcheck results instead of editing the existing one? (and post a comment saying "No lintcheck changes detected" where there are no lintcheck changes)
@@ -24,7 +24,8 @@ permissions: | |||
jobs: | |||
download: | |||
runs-on: ubuntu-latest | |||
if: ${{ github.event.workflow_run.conclusion == 'success' }} | |||
# FIXME(feature freeze): Make sure to remove that last part after the feature freeze is over | |||
if: ${{ github.event.workflow_run.conclusion == 'success' && ! contains(github.event.pull_request.labels.*.name, 'A-new-lint') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the lint is A-lint
.
It would be a better fix, but the gh cli doesn't put it easy when it comes to editing comments when they're not the last one.
With this approach (more suited to the temporary nature that the feature freeze has), we run the risk of spamming PRs. I'm not sure what is worse in this scenario. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label needs to be correctly named A-lint
, not A-new-lint
. Did you test this on a fork or a dummy PR? Would be nice to do so, before merging.
But generally ✔️ .
Correct label name
89b023d
to
f065537
Compare
Make sure that lintcheck summary comment do not overwrite PRs with the
A-new-lint
label that is applied on freezed PRs. As the lintcheck summary action runs after conclusion and after lintcheck, it's impossible that it gets executed before applying the label.(Also, those spaces I added are not an accident, GHA treats empty lines very weirdly)
r? @flip1995
changelog: none